Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix: move config validation to startup stage so bad configs will break the component #425

Merged
merged 2 commits into from
Feb 13, 2024
Merged

fix: move config validation to startup stage so bad configs will break the component #425

merged 2 commits into from
Feb 13, 2024

Conversation

robcmann
Copy link
Member

Description of changes:
Move config subscription and validation from install to startup stage.

Why is this change necessary:
Moving the config validation to the startup stage will prevent CDA from ever reaching the RUNNING state if the config is bad.

How was this change tested:

Any additional information or context required to review the change:

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

MikeDombo
MikeDombo reviewed Feb 13, 2024
View reviewed changes
src/main/java/com/aws/greengrass/clientdevices/auth/ClientDevicesAuthService.java
@@ -214,6 +213,7 @@ private void configChangeHandler(WhatHappened whatHappened, Node node) {
@Override
protected void startup() throws InterruptedException {
context.get(CertificateManager.class).startMonitors();
subscribeToConfigChanges();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

subscribe registers a config change callback. That callback needs to be saved as a final field so that we don't duplicate callbacks

@github-actions GitHub Actions
Copy link

github-actions bot commented Feb 13, 2024 •
edited
Loading

Code Coverage Report

File Coverage
All files 74% ✅
com.aws.greengrass.clientdevices.auth.PermissionEvaluationUtils 90% ✅
com.aws.greengrass.clientdevices.auth.CertificateManager 88% ✅
com.aws.greengrass.clientdevices.auth.ClientDevicesAuthService 82% ✅
com.aws.greengrass.clientdevices.auth.DeviceAuthClient 73% ✅
com.aws.greengrass.clientdevices.auth.certificate.ClientCertificateGenerator 95% ✅
com.aws.greengrass.clientdevices.auth.certificate.CertificateHelper 74% ✅
com.aws.greengrass.clientdevices.auth.certificate.CertificateStore 72% ✅
com.aws.greengrass.clientdevices.auth.certificate.CertificateExpiryMonitor 87% ✅
com.aws.greengrass.clientdevices.auth.certificate.ServerCertificateGenerator 93% ✅
com.aws.greengrass.clientdevices.auth.certificate.CertificateGenerator 70% ✅
com.aws.greengrass.clientdevices.auth.certificate.CertificateExpiryMonitor$CertRotationDecider 90% ✅
com.aws.greengrass.clientdevices.auth.iot.usecases.VerifyIotCertificate 94% ✅
com.aws.greengrass.clientdevices.auth.iot.usecases.VerifyThingAttachedToCertificate 89% ✅
com.aws.greengrass.clientdevices.auth.iot.usecases.CreateIoTThingSession 67% ✅
com.aws.greengrass.clientdevices.auth.iot.usecases.VerifyCertificateValidityPeriod 88% ✅
com.aws.greengrass.clientdevices.auth.certificate.infra.BackgroundCertificateRefresh 80% ✅
com.aws.greengrass.clientdevices.auth.iot.infra.ThingRegistry 92% ✅
com.aws.greengrass.clientdevices.auth.certificate.usecases.ConfigureManagedCertificateAuthority 85% ✅
com.aws.greengrass.clientdevices.auth.certificate.usecases.ConfigureCustomCertificateAuthority 83% ✅
com.aws.greengrass.clientdevices.auth.certificate.usecases.RegisterCertificateAuthorityUseCase 65% ✅
com.aws.greengrass.clientdevices.auth.configuration.MetricsConfiguration 83% ✅
com.aws.greengrass.clientdevices.auth.configuration.GroupManager 94% ✅
com.aws.greengrass.clientdevices.auth.configuration.CAConfiguration 96% ✅
com.aws.greengrass.clientdevices.auth.configuration.RuntimeConfiguration 83% ✅
com.aws.greengrass.clientdevices.auth.configuration.SecurityConfiguration 80% ✅
com.aws.greengrass.clientdevices.auth.configuration.GroupDefinition 75% ✅
com.aws.greengrass.clientdevices.auth.configuration.ExpressionVisitor 84% ✅
com.aws.greengrass.clientdevices.auth.configuration.GroupConfiguration 92% ✅
com.aws.greengrass.clientdevices.auth.api.ClientDevicesAuthServiceApi 85% ✅
com.aws.greengrass.clientdevices.auth.api.GetCertificateRequestWithGenerator 75% ✅
com.aws.greengrass.clientdevices.auth.api.UseCases 71% ✅
com.aws.greengrass.clientdevices.auth.session.attribute.WildcardSuffixAttribute 96% ✅
com.aws.greengrass.clientdevices.auth.iot.IotAuthClient$Default 56% ✅
com.aws.greengrass.clientdevices.auth.iot.Thing 85% ✅
com.aws.greengrass.clientdevices.auth.iot.Certificate 74% ✅
com.aws.greengrass.clientdevices.auth.iot.GreengrassV2DataClientFactory 18% ❌
com.aws.greengrass.clientdevices.auth.iot.CertificateRegistry 95% ✅
com.aws.greengrass.clientdevices.auth.infra.NetworkStateProvider$Default 83% ✅
com.aws.greengrass.ipc.IPCUtils 83% ✅
com.aws.greengrass.ipc.VerifyClientDeviceIdentityOperationHandler 60% ✅
com.aws.greengrass.ipc.GetClientDeviceAuthTokenOperationHandler 83% ✅
com.aws.greengrass.ipc.AuthorizeClientDeviceActionOperationHandler 79% ✅
com.aws.greengrass.ipc.SubscribeToCertificateUpdatesOperationHandler 81% ✅
com.aws.greengrass.clientdevices.auth.session.SessionConfig 92% ✅
com.aws.greengrass.clientdevices.auth.session.SessionManager 88% ✅
com.aws.greengrass.clientdevices.auth.certificate.handlers.CAConfigurationChangedHandler 93% ✅
com.aws.greengrass.clientdevices.auth.certificate.handlers.CertificateRotationHandler 96% ✅
com.aws.greengrass.clientdevices.auth.metrics.handlers.SessionCreationEventHandler 88% ✅
com.aws.greengrass.clientdevices.auth.metrics.handlers.MetricsConfigurationChangedHandler 70% ✅
com.aws.greengrass.clientdevices.auth.metrics.handlers.AuthorizeClientDeviceActionsMetricHandler 88% ✅
com.aws.greengrass.clientdevices.auth.metrics.handlers.VerifyClientDeviceIdentityEventHandler 88% ✅
com.aws.greengrass.clientdevices.auth.metrics.handlers.CertificateSubscriptionEventHandler 83% ✅
com.aws.greengrass.clientdevices.auth.util.ResizableLinkedBlockingQueue 90% ✅
com.aws.greengrass.clientdevices.auth.util.ParseIPAddress 90% ✅
com.aws.greengrass.clientdevices.auth.connectivity.CISShadowMonitor$SucceedOnceOperation 88% ✅
com.aws.greengrass.clientdevices.auth.connectivity.CISShadowMonitor 79% ✅
com.aws.greengrass.clientdevices.auth.connectivity.CISShadowMonitor$ProcessCISShadowTask 76% ✅
com.aws.greengrass.clientdevices.auth.connectivity.CISShadowMonitor$CISShadowTaskQueue 82% ✅
com.aws.greengrass.clientdevices.auth.connectivity.HostAddress 67% ✅
com.aws.greengrass.clientdevices.auth.connectivity.CISShadowMonitor$CISShadowTaskExecutor 80% ✅
com.aws.greengrass.clientdevices.auth.configuration.parser.TokenMgrError 22% ❌
com.aws.greengrass.clientdevices.auth.configuration.parser.RuleExpressionTokenManager 61% ✅
com.aws.greengrass.clientdevices.auth.configuration.parser.ASTStart 33% ❌
com.aws.greengrass.clientdevices.auth.configuration.parser.ASTAnd 67% ✅
com.aws.greengrass.clientdevices.auth.configuration.parser.Token 58% ✅
com.aws.greengrass.clientdevices.auth.configuration.parser.RuleExpressionDefaultVisitor 0% ❌
com.aws.greengrass.clientdevices.auth.configuration.parser.ASTOr 67% ✅
com.aws.greengrass.clientdevices.auth.configuration.parser.SimpleCharStream 28% ❌
com.aws.greengrass.clientdevices.auth.configuration.parser.RuleExpressionTreeConstants 0% ❌
com.aws.greengrass.clientdevices.auth.configuration.parser.JJTRuleExpressionState 67% ✅
com.aws.greengrass.clientdevices.auth.configuration.parser.ASTThing 67% ✅
com.aws.greengrass.clientdevices.auth.configuration.parser.RuleExpression 63% ✅
com.aws.greengrass.clientdevices.auth.configuration.parser.SimpleNode 27% ❌

Minimum allowed coverage is 50%

Generated by 🐒 cobertura-action against 179c901

@github-actions GitHub Actions
Copy link

github-actions bot commented Feb 13, 2024 •
edited
Loading

Benchmark Results

Benchmark Score
com.aws.greengrass.clientdevices.auth.benchmark.AuthorizationBenchmarks.GIVEN_policy_with_thing_name_variable_WHEN_auth_request_THEN_successful_auth 1277342.73 ops/s
com.aws.greengrass.clientdevices.auth.benchmark.AuthorizationBenchmarks.GIVEN_policy_with_wildcards_WHEN_auth_request_THEN_successful_auth 209586.63 ops/s
com.aws.greengrass.clientdevices.auth.benchmark.AuthorizationBenchmarks.GIVEN_single_group_permission_WHEN_simple_auth_request_THEN_successful_auth 2551093.42 ops/s

@robcmann robcmann marked this pull request as ready for review February 13, 2024 22:06
@robcmann robcmann merged commit 0f8a25c into main Feb 13, 2024
5 checks passed
@robcmann robcmann deleted the config-validation branch February 13, 2024 22:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jcosentino11 jcosentino11 jcosentino11 approved these changes

@MikeDombo MikeDombo MikeDombo approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@robcmann @jcosentino11 @MikeDombo